test: Repro 'dune fmt' crash in presence of Nix result #11202
Conversation
Julow
force-pushed
the
nix-result-fmt-crash
branch
from
eeb7511 to
a228ff9
Compare
There was a problem hiding this comment.
I have seen this before and it can be a bit puzzling.
IMO the issue here isn't that dune is following symlinks, but rather that the files in the nix store /nix/store/... have pretty locked-down permissions as one would expect.
I'm not saying that dune is perfect, just trying to point out the actual cause. There might be work to be done in dune itself.
|
Is here a case where we want the files in the symlinked directory be formatted ? |
| Execution will pass over me and through me. And when it has gone past, I | ||
| will unwind the stack along its path. Where the cases are handled there will | ||
| be nothing. Only I will remain. | ||
| [1] |
There was a problem hiding this comment.
Could you filter the output a bit so it is more reproducible? The locations in the stacktrace are probably going to change over time. A simple grep EACCES is probably sufficient to show there's a very specific error that happens.
There was a problem hiding this comment.
I hopted to remove the output and rely on the exit status.
Julow
force-pushed
the
nix-result-fmt-crash
branch
from
9f4055b to
be4fc2b
Compare
Leonidas-from-XIV
force-pushed
the
nix-result-fmt-crash
branch
from
be4fc2b to
2f13de7
Compare
A 'ocamlformat' executable is added to the test to avoid depending on OCamlformat. Signed-off-by: Jules Aguillon <[email protected]>
Make user errors in hooks just regular errors Simplify the erorr for promotion failures Signed-off-by: Rudi Grinberg <[email protected]>
rgrinberg
force-pushed
the
nix-result-fmt-crash
branch
from
2f13de7 to
029d26e
Compare
|
Thanks. This is just required a little more code for a better error message. |
| _build/default/result/.formatted/foo.ml differ. | ||
| Promoting _build/default/result/.formatted/foo.ml to result/foo.ml. | ||
| Error: failed to promote result/foo.ml | ||
| Permission denied |
There was a problem hiding this comment.
Thanks for the fix! With this output, it's clearer that removing result would fix the issue.
But couldn't Dune avoid this issue ? Formatting files that are outside the workspace is surprising but could be part of someone's workflow. However, formatting files that are read-only feels like something Dune could avoid without bothering anyone.
There was a problem hiding this comment.
However, formatting files that are read-only feels like something Dune could avoid without bothering anyone.
That's an interesting suggestion. It does seem like an improvement for the current behavior. There's no need to setup rules if we know they're good to fail. Do you want to try implementing this?
There was a problem hiding this comment.
I've implemented that in #11490 but I have the feeling that is not the right solution. result links might interfere with other things than formatting and I think they should not be read by Dune.
Reproduction case for a crash when using
dune fmt.Perhaps formatting shouldn't follow symlinks ? Also,
dune builddoesn't seem to have any problem with that, perhaps because the linked directory doesn't contain a dune file.Perhaps symlinked directories should be considered
vendored_dirsautomatically ?